Skip to content

Conversation

@esezen
Copy link
Contributor

@esezen esezen commented Jan 12, 2026

If a consumer doesn't import styles, left arrow points to the right. This PR removes that dependency

Before:
CleanShot 2026-01-12 at 14 19 18@2x

After:
CleanShot 2026-01-12 at 14 18 40@2x

Copilot AI review requested due to automatic review settings January 12, 2026 11:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where the left arrow icon pointed in the wrong direction when Tailwind styles were not imported. The fix replaces the CSS-rotation-based approach with a native SVG implementation.

Changes:

  • Removed dependency on Tailwind's rotate-180 class for the left arrow icon
  • Implemented ArrowLeftIcon as a standalone SVG instead of a rotated ArrowRightIcon
  • Updated vertical carousel rotation to use rotate-90 instead of -rotate-90

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/assets/icons/ArrowLeftIcon.tsx Replaced CSS rotation logic with native left-pointing SVG arrow
src/components/carousel.tsx Fixed vertical orientation rotation from -rotate-90 to rotate-90

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@constructor-claude-bedrock
Copy link

Code Review Results

✅ Strengths

The PR successfully removes the Tailwind CSS dependency for arrow icons by implementing native SVG icons, addressing the reported bug where arrows pointed incorrectly when styles weren't imported.

🚨 Critical Issues

[File: src/assets/icons/ArrowLeftIcon.tsx Line: L7]
The stroke color is hardcoded to #000 (black), which prevents customization and creates accessibility issues. The icon should respect the parent component's color or accept it as a prop.

// Current implementation:
stroke='#000'

// Recommended:
stroke='currentColor'

This allows the icon to inherit the text color from its parent, making it themeable and accessible.

⚠️ Important Issues

[File: src/components/carousel.tsx Line: L328]
The rotation direction was changed from -rotate-90 to rotate-90 for vertical orientation of the left arrow. While this appears intentional given the switch from a transformed right arrow to a native left arrow, this change should be verified to ensure the arrow points in the correct direction for vertical carousels. The original implementation rotated the right arrow -90 degrees (counterclockwise) for upward navigation, but now it rotates the left arrow +90 degrees (clockwise).

[General: Missing Test Coverage]
According to the review guidelines (section "Quality Validation"), changes should be covered with tests. The PR modifies the icon implementation and carousel rotation logic but does not include:

  • Tests verifying the ArrowLeftIcon renders correctly as an independent SVG
  • Visual regression tests or snapshot tests to ensure the icons display as expected
  • Tests verifying the rotation behavior for vertical orientation still works correctly

💡 Suggestions

[File: src/assets/icons/ArrowLeftIcon.tsx Line: L5]
Consider adding default props for better consistency:

const ArrowLeftIcon = ({ 
  width = 16, 
  height = 16, 
  ...props 
}: SVGProps<SVGSVGElement>) => (
  <svg 
    xmlns='http://www.w3.org/2000/svg' 
    width={width} 
    height={height} 
    fill='none' 
    {...props}
  >

This maintains the default size while allowing overrides when needed.

[File: src/assets/icons/ArrowLeftIcon.tsx Line: L7]
Consider making strokeWidth customizable through props or CSS variables to maintain consistency with design system tokens:

<path
  stroke='currentColor'
  strokeLinecap='round'
  strokeLinejoin='round'
  strokeWidth='var(--icon-stroke-width, 2)'
  d='m10 12-4-4 4-4'
/>

Overall Assessment: ⚠️ Needs Work

The PR successfully addresses the core issue but requires fixing the hardcoded stroke color (critical for theming/accessibility) and adding test coverage to prevent regressions.

Copy link
Contributor

@Alexey-Pavlov Alexey-Pavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@esezen esezen merged commit 9f77116 into main Jan 20, 2026
9 checks passed
@esezen esezen deleted the cdx-355-shared-components-bugfix-make-arrows-work-without-depending branch January 20, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants